Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Oct 3, 2020

This PR removes (most of) the special case pretty printing code in DataFusion's sql integration test, sql.rs in favor of the standard pretty printer in arrow::utils::pretty and in the process adds support for DictionaryArray printing as well as standardizing how tests are run and output is compared.

I am working on adding support for DictionaryArray in DataFusion (and thus want to add tests to sql.rs). This specific PR's changes are larger than strictly necessary, but I felt it made it easier to write new tests in sql.rs. However, if people prefer, I could instead add a special case for DictionaryArray in array_str and accomplish my goal with a much smaller PR

Note: I found that using Vec<Vec<String>> to encode expected results rather than a String retains the nice property that differences to expected output are shown reasonably in the test output. For example:

---- csv_query_cast_literal stdout ----
thread 'csv_query_cast_literal' panicked at 'assertion failed: `(left == right)`
  left: `[["0.9294097332465232", "1.0"], ["0.3114712539863804", "1.0"]]`,
 right: `[["0.9294097332465232", "1"], ["0.3114712539863804", "1"]]`', datafusion/tests/sql.rs:502:5

@github-actions
Copy link

github-actions bot commented Oct 3, 2020

@alamb alamb changed the title ARROW-10159: [Rust] [DataFusion] Support DictionaryArray in sql.rs tests, by using standard pretty printer ARROW-10167: [Rust] [DataFusion] Support DictionaryArray in sql.rs tests, by using standard pretty printer Oct 4, 2020
@alamb alamb force-pushed the alamb/ARROW-10167-cleanup-sql-display branch from 0b2dd5c to 9d22a54 Compare October 4, 2020 11:00
@github-actions
Copy link

github-actions bot commented Oct 4, 2020

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a pretty good example of the change in structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this type was missing in pretty.rs (I found it while converting the tests in sql.rs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did cheat somewhat here and punt on adding proper ListArray support to pretty.rs.

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alamb this is a nice cleanup of the tests.

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice, @alamb . LGTM

@jorgecarleitao
Copy link
Member

@alamb we need a small rebase to merge.

@alamb alamb force-pushed the alamb/ARROW-10167-cleanup-sql-display branch from da3aec1 to 177c6ce Compare October 6, 2020 09:37
@alamb
Copy link
Contributor Author

alamb commented Oct 6, 2020

@jorgecarleitao FYI rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants